You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
The convertedArray variable on line 207 is created but then not used in the return statement on line 208, which creates a new array conversion instead.
In several nullable type conversion methods (lines 103-156), the parameter name 'b' is used for the unwrapped value, which is inconsistent with the parameter type (should use more descriptive names like 'intValue' instead of 'b' for integers).
✅ Remove redundant array conversionSuggestion Impact:The commit completely removed the problematic method ConvertFrom(object?[]?) that contained the redundant array conversion. Instead of fixing the specific issue, the commit refactored the code to use generic methods, eliminating the need for multiple specialized methods including the one with the redundant conversion.
code diff:
- public static LocalValue ConvertFrom(object?[]? values)- {- if (values is null)- {- return new NullLocalValue();- }-- LocalValue[] convertedArray = Array.ConvertAll(values, ConvertFrom);- return new ArrayLocalValue(Array.ConvertAll(values, ConvertFrom));- }
The method is converting the array twice unnecessarily. The convertedArray variable is created but never used, and then the conversion is performed again in the return statement.
public static LocalValue ConvertFrom(object?[]? values)
{
if (values is null)
{
return new NullLocalValue();
}
LocalValue[] convertedArray = Array.ConvertAll(values, ConvertFrom);
- return new ArrayLocalValue(Array.ConvertAll(values, ConvertFrom));+ return new ArrayLocalValue(convertedArray);
}
[Suggestion has been applied]
Suggestion importance[1-10]: 9
__
Why: The suggestion correctly identifies a significant bug where the array is converted twice unnecessarily. The code creates a convertedArray variable but then ignores it, performing the conversion again in the return statement. This wastes CPU cycles and memory, potentially impacting performance for large arrays.
Our big concern here is whether we should support nullable primitive types. I guess it is more philosophic question, from implementation perspective for us it doesn't matter. For end users it is also kind of "doesn't matter".
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
Description
Add strongly-typed
LocalValue.ConvertFromoverloadsMotivation and Context
Prevents users from being forced to guess which types we happen to support.
Types of changes
Checklist
PR Type
Enhancement
Description
Added strongly-typed
LocalValue.ConvertFromoverloads for better type safety.Refactored
ConvertFromlogic to delegate to specific type overloads.Introduced new methods for handling various data structures and primitives.
Improved code maintainability and readability by reducing duplication.
Changes walkthrough 📝
LocalValue.cs
Refactored and extended `LocalValue.ConvertFrom` functionalitydotnet/src/webdriver/BiDi/Modules/Script/LocalValue.cs
ConvertFrommethod.ConvertFrommethods.collections.
structure.